Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DBAL-1130

We use Jira to track the state of pull requests and the versions they got
included in.

Here's the proof that the test is causing an exception, but only because the password is incorrect - not because there's some sort of "Connection is Closed" exception: https://travis-ci.org/doctrine/dbal/jobs/48084218#L298

Agree that the test should be rewritten somehow. The test is very risky and highly depends on implementation detail. I guess either Reflection has to be used to test the internal state of Connection::$_conn or it has to be moved to the functional test suite.
Speaking of that, the credentials defined in this test class are only there so that the driver / driver manager does not throw an exception because of missing params. In the end this test class is not intended to do real database interaction.

@deeky666 I agree on all points! Now that we're connecting when the platform is being determined, we do now try to connect in some cases, so we have to be careful (since, as you said, it was never the real intention). I'd really like your thoughts on #783 (other than the removal of this test in that PR - I can rewrite it with reflection).

Thanks!

Done! Summary:

1. This test was introduced for Fixed closing a connection #688 in Add test case for #688 #691.
2. My implementation is taken from one of the attempts on Add test case for #688 #691
3. I have properly tested that the test DOES fail if the inner _conn is not nullified

The only issue is that this test should technically test that the property was nullified, and not unset. But, I'm not actually sure if that's testable - even with reflection. Even when I unset the property temporarily, Reflection sees it just fine (since it's in the class declaration).

My opinion (fwiw) is that we merge this, or do-away with the test entirely. But I think we should keep it.

Thanks!

@weaverryan thanks for your investigation! IMO if not even reflection can detect whether a property was unset, this test is rather useless. As you stated this test does not fail if the property was unset so it is not a failing test case for the original issue.
Another idea I have is to create a mock class for this particular test case which extends the connection class and adds an additional method that directly accesses $this->_conn like this:

class DBAL1003Connection extends \Doctrine\DBAL\Connection
{
    public function getWrappedConnection()
    {
        return $this->_conn;
    }
}

class ConnectionTest extends \Doctrine\Tests\DbalTestCase
{
    $connection = new DBAL1003Connection(); // params to be inserted

    $connection->close();

    $this->assertNull($connection->getWrappedConnection());
}

I think this should raise the notice and reveal the bug.

Or we simply mock Doctrine\DBAL\Connection and make connect() do nothing. This could have the same effect.

Yep that reveals the issue:

public function testConnectionIsClosed()
{
    $connection = $this->getMockBuilder('Doctrine\DBAL\Connection')
        ->disableOriginalConstructor()
        ->setMethods(array('connect'))
        ->getMock();

    $connection->close();

    $this->assertNull($connection->getWrappedConnection());
}

@deeky666 excellent suggestion - I've updated the test (I'm still using reflection to set _conn internally - then this will fail if _conn is unset or if we forget to set it to null on close()).

I've verified that unset($this->_conn) or forgetting $this->_conn = null both make the test fail now.

Thanks!

@weaverryan finally merging, thanks again!